Skip to content

Duck pond!#621

Merged
lemonsaurus merged 29 commits into
masterfrom
duck_pond
Nov 30, 2019
Merged

Duck pond!#621
lemonsaurus merged 29 commits into
masterfrom
duck_pond

Conversation

@lemonsaurus
Copy link
Copy Markdown
Contributor

@lemonsaurus lemonsaurus commented Oct 27, 2019

This cog will listen for duck reactions on any message, and then:

  • If the reaction was added by a staff member
  • and the reaction was a duck
  • and the message has not already been added to the #duck-pond

It will add the message to the #duck-pond channel and then add a green checkbox to the original message to indicate that the message has been ponded undergone pondification. Once this checkmark has been added, the message will not be processed in the future. If the checkmark is removed and there are more than ducks_required ducks on the message, the bot will automatically add the checkmark back.

Messages are added to the #duck-pond via webhook, so that they can retain the appearance of having their original authors.

image

If all reactions are removed (with the clear all reactions action), the bot does not have a countermeasure for this. In order to implement a countermeasure, it would be necessary to involve the API and the database. This might be an interesting change to make for a future version of this, because it would also allow us to do stuff like keep track of how many ducks are associated with each post, and create leaderboards -- but it's out of scope for this initial PR.

Leon Sandøy added 2 commits October 27, 2019 02:50
This adds the emojis, the channel, and the configuration needed
for the duck-pond feature. This is added both to config-default.yml,
and to the constants.py file.
This cog will listen for duck reactions on any message, and then:
- If the reaction was added by a staff member
- and the reaction was a duck
- and the message has not already been added to the #duck-pond

It will add the message to the #duck-pond and then add a green checkbox
to the original message to indicate that the message has been ponded.

Messages are added to the #duck-pond via webhook, so that they can
retain the appearance of having their original authors.

Once this checkmark has been added, the message will not be processed in
the future.

If the checkmark is removed and there are more than ducks_required ducks
on the message, the bot will automatically add the checkmark back.

However, if all reactions are removed, the bot does not have a
countermeasure for this. In order to implement a countermeasure, it
would be necessary to involve the API and the database.
@lemonsaurus lemonsaurus changed the title Duck pond! 🦆 Duck pond! Oct 27, 2019
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/constants.py
Co-Authored-By: Mark <kozlovmark@gmail.com>
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Comment thread bot/constants.py Outdated
Comment thread bot/cogs/duck_pond.py Outdated
Leon Sandøy added 3 commits October 27, 2019 15:16
This refactors the duck pond cog to have fewer redundancies,
removes some unused features (like supporting reaction_list in
the count_duck and has_green_checkbox helpers), and makes other
various minor (mostly cosmetic) improvements.
@lemonsaurus
Copy link
Copy Markdown
Contributor Author

All review comments are addressed, but I guess I should probably write tests for this feature, so don't merge it yet.

@MarkKoz MarkKoz added area: cogs t: feature New feature or request labels Oct 27, 2019
Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Putting a more "formal" request for unittests, just in case...

@lemonsaurus
Copy link
Copy Markdown
Contributor Author

I started on the tests, will try to finish them later tonight.

Leon Sandøy and others added 9 commits October 31, 2019 07:39
This adds empty tests for all the tests I'd like to add to this
pull request. It also adds a few more duckies to the emoji
constant list, and adds a single line of clarification to the
testing readme.
Also gets started setting up for the final tests, which will require
more mockwork.
Basically I suck at this and I can't get this return_value thing to work.
I'll have Ves look at it to resolve it.

As of right now, multiple tests are failing.
By default, a mocked value is considered `truthy` in Python, like all
non-empty/non-zero/non-None values in Python. This means that if an
attribute is not explicitly set on a mock, it will evaluate at as
truthy in a boolean context, since the mock will provide a truthy
mocked value by default.

This is not the best default value for the `bot` attribute of our
MockMember type, since members are rarely bots. It makes much more
intuitive sense to me to consider a member to not be a bot, unless we
explicitly set `bot=True`.

This commit sets that sensible default value that can be overwritten
by passing `bot=False` to the constructor or setting the `object.bot`
attribute to `False` after the creation of the mock.
As stated from the start, our intention is to add custom mock types
as we need them for testing. While writing tests for DuckPond, I
noticed that we did not have a mock type for Attachments, so I added
one with this commit.

In addition, I think it's a very sensible for MockMessage to have an
empty list as a default value for the `attachements` attribute. This
is equal to what `discord.Message` returns for a message without
attachments and makes sure that if you don't explicitely add an
attachment to a message, `MockMessage.attachments` tests as falsey.
Previously, the presence of any green checkmark as a reaction would
prevent a message from being relayed to the duck pond, regardless of
the actor of that reaction. Since we only want to check if the bot
has already processed this message, we should check for a checkmark
added by the bot.

This commit adds such a user check.
To allow for separate testing of the code that relays messages to the
duck pond, I have moved this part of the code from the event listener
to a separate method. The overall logic has remained unchanged.

In addition, I've kaizened to things:

- Removed unnecessary f-string without interpolation;

- Removed double negative (not item not in list)
The `DuckPond.on_raw_message_add` event listener makes an API call to
fetch the message the reaction was added to. However, we don't need
to fetch the message if the reaction that was added is not relevant
to the duck pond. To prevent such unnecessary API calls, I have moved
the code that checks for the relevance of the reaction event to
before the code that fetches the message.
The AsyncIteratorMock included in Python 3.8 will work similarly to
the mocks of callabes. This means that it allows you to set the items
it will yield using the `return_value` attribute. It will also have
support for the common Mock-specific assertions.

This commit introduces some backports of those features in a slightly
simplified way to make the transition to Python 3.8 easier in the
future.
I have added a special mock that follows the specifications of a
`discord.User` instance. This is useful, since `Users` have less
attributes available than `discord.Members`. Since this difference
in availability of information can be important, we should not use
a `MockMember` to mock a `discord.user`.
The new AsyncIteratorMock no longer needs an additional method to be
used with a Mock object.
I have added a mock type to mock `discord.Webhook` instances. Note
that the current type is specifically meant to mock webhooks that
use an AsyncAdaptor and therefore has AsyncMock/coroutine mocks for
the "maybe-coroutine" methods specified in the `discord.py` docs.
This commit adds unit tests that provide a full branch coverage of
the `bot.cogs.duck_pond` file.
@SebastiaanZ
Copy link
Copy Markdown
Contributor

I just realized that I did not yet add tests for the new mock types I've added. I'll do that tomorrow, sleep first.

Copy link
Copy Markdown
Contributor

@MarkKoz MarkKoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So far looks good. Have gotten to everything except a692a95 though; will review the rest tomorrow or on the weekend.

Comment thread tests/helpers.py
@scragly scragly added a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 3 - low Low Priority labels Nov 15, 2019
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
(constants.DuckPond.threshold, True),
(constants.DuckPond.threshold + 1, True),
)
for duck_count, should_readd_checkmark in test_cases:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the second d in readd have a meaning ("d" for "ducks"?) or is it a typo?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change this to re_add to make it clear we're talking about re-adding the emoji.

Comment thread tests/bot/cogs/test_duck_pond.py Outdated

with self.assertLogs(logger=log, level=logging.INFO) as log_watcher:
duck_pond.setup(bot)
line = log_watcher.output[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this raise an IndexError if it had no log messages? That would seem undesirable compared to an assertion failure.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes, I failed to rewrite this.

Comment thread bot/cogs/duck_pond.py Outdated
duck_reactors.append(user.id)
return duck_count

async def relay_message_to_duck_pond(self, message: Message) -> None:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name feels too verbose. How about relay_messages?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay with me. I liked the verbosity when reading the name, as it spells out what's going to happen at that point in the code and relay_message feels more generic (which this method is not). It doesn't really matter, though, so I'll change it.

Comment thread bot/cogs/duck_pond.py
Co-Authored-By: Mark <kozlovmark@gmail.com>
Comment thread tests/bot/cogs/test_duck_pond.py
Comment thread tests/bot/cogs/test_duck_pond.py Outdated
Comment thread tests/bot/cogs/test_duck_pond.py
I moved the check that tests if a payload contains a duck emoji to a
separate method. This makes it easier to test this part of the code
as a separate unit than when it's contained in the larger event
listener.

In addition, I kaizened the name `relay_message_to_duckpond` to the
less verbose `relay_message`; that's already clear enough.
#621

I've changed to unit tests according to the comments made on the
issue. Most changes are straightforward enough, but, for context,
see the PR linked above.
@lemonsaurus
Copy link
Copy Markdown
Contributor Author

I'mma just merge this, it's gotten plenty of reviews and it's far too well tested that it could possibly break.

@lemonsaurus lemonsaurus merged commit d7a2087 into master Nov 30, 2019
@lemonsaurus lemonsaurus deleted the duck_pond branch November 30, 2019 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: utility Related to utility commands: (bot, eval, extensions, jams, reminders, snekbox, utils) p: 3 - low Low Priority t: feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants